[connectors] Google drive parent scripts improvement #9390
Conversation
tdraier
changed the title
| }, | ||
| order: [["id", "ASC"]], | ||
| limit: QUERY_BATCH_SIZE, | ||
| } | ||
| ); | ||
|
|
||
| for (const file of googleDriveFiles) { | ||
| loop: for (const file of googleDriveFiles) { |
There was a problem hiding this comment.
is that a label?? wow :)
There was a problem hiding this comment.
I did not use labels for years :-D
| }, | ||
| order: [["id", "ASC"]], | ||
| limit: QUERY_BATCH_SIZE, | ||
| } | ||
| ); | ||
|
|
||
| for (const file of googleDriveFiles) { | ||
| loop: for (const file of googleDriveFiles) { |
There was a problem hiding this comment.
nit: rename googleDriveFiles as googleDriveFolders since in that case we select for that
There was a problem hiding this comment.
I changed again and now gettng folders and csv , so wil keep "files" :-)
| const internalId = file.dustFileId; | ||
| const driveFileId = file.driveFileId; | ||
| const parents = [driveFileId]; | ||
| let current: string | null = file.parentId; | ||
| while (current) { | ||
| parents.push(current); | ||
| if (typeof parentsMap[current] === "undefined") { |
There was a problem hiding this comment.
aren't we guaranteed to err at some point? since at some point we get to the root drive, which is not in googleDriveFiles right?
YPATH so I'm likely missing something there 😅
There was a problem hiding this comment.
the root dirve is indeed in googleDriveFiles, so in most of the case we don't err :-)
But I found many places where we have parentId which do not match any google_drive_file (in that case I can't guess the parents list)
| @@ -146,7 +122,7 @@ async function migrate({ | |||
| limit: QUERY_BATCH_SIZE, | |||
| }); | |||
|
|
|||
| for (const sheet of googleDriveSheets) { | |||
| loop: for (const sheet of googleDriveSheets) { | |||
There was a problem hiding this comment.
nit: Honestly a bit wary about the loop, always a bit hard to read
i'd have a small function computeParents instead of the while, and have it return undefined in error cases continuing without a label (you could also reuse it on docs)
but it's a nit so feel free to ignore
There was a problem hiding this comment.
ok I was not very proud of the label things .. extracting a function
| while (current) { | ||
| parents.push(current); | ||
| current = parentsMap[current] || null; | ||
| const parents = getParents(file.parentId, driveFileId, parentsMap); |
There was a problem hiding this comment.
hard to understand here; getParents take a fileId and a nodeId, the fileId should be the first parent, the nodeId should be the child, and we get the parents for the child but without the child as parents[0] which is added later, is that it? at a minimum, args should be renamed for this to be clear
would it be possible to make a single function getParents(nodeId, parentsMap) that would return the exact parents array we want, and minimize the unshifts we do later?
There was a problem hiding this comment.
agree the params are unclear, but we can't just use one single nodeId param, because if nodeId is a spreadsheet id it won't be in the parentsMap (which contains only files). The nodeId param is just here for logging.
There was a problem hiding this comment.
replaced nodeId with logger param
Description
Risk
Deploy Plan